Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addon-knobs: Add disableForceUpdate option #9447

Merged
merged 5 commits into from
Feb 21, 2020
Merged

Conversation

novli
Copy link
Contributor

@novli novli commented Jan 14, 2020

Issue: #9128

What I did

Added an option 'disableForceUpdate' to disable force-updating by changed knobs.

How to test

I've managed to use knobs utility like this:

const useComponentState = () => {
  const [itemId, setChosen] = useState(text('ItemId', '2'));
  useEffect(() => {
    const onChange = (change: any) => {
      if (!change.manual) {
        if (change.name === 'ItemId') {
          setChosen(change.value);
        }
      }
    };
    addons.getChannel().addListener('storybookjs/knobs/change', onChange);
    return () => {
      addons.getChannel().removeListener('storybookjs/knobs/change', onChange);
    };
  }, []);
  const onClick = useCallback(
    (oldId, newId, event) => {
      if (oldId !== newId) {
        setChosen(newId);
        addons
          .getChannel()
          .emit('storybookjs/knobs/change', { name: 'ItemId', value: newId, manual: true });
        action('onClick')(oldId, newId, event);
      }
    },
    [],
  );
  return { itemId, onClick };
};

const Component = () => {
  const { itemId, onClick } = useComponentState();
  return (
    <Menu
      items={Items}
      itemId={itemId}
      onClick ={onClick }
    />
  );
};

storiesOf('Components', module)
  .addDecorator(withKnobs({ disableForceUpdate: true }))
  .add('<Menu />', Component);

So I can use both knobs and setState to manage component state. The only problem is that the knobs change always re-renders the whole component. I don't need this behavior, because I subscribe to knobs manually and re-render component by React itself.

The requested change won't break anything, but it will suppose a way around re-rendering issues like this #9128.

  • Is this testable with Jest or Chromatic screenshots? No.
  • Does this need a new example in the kitchen sink apps? No.
  • Does this need an update to the documentation? No.

@vercel
Copy link

vercel bot commented Jan 14, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

@vercel vercel bot temporarily deployed to Preview January 18, 2020 22:27 Inactive
@novli
Copy link
Contributor Author

novli commented Jan 18, 2020

The code has slightly changed. Now it is allowed to change this behavior by knob options like this:

    knob('Enabled', {
      type: 'boolean',
      value: true,
      groupId: 'Component',
      disableForceUpdate: true,
      disableDebounce: true,
    }),

@wovalle
Copy link

wovalle commented Jan 22, 2020

Exactly what I'm looking for! hopefully this gets merged soon!

@shilman
Copy link
Member

shilman commented Jan 23, 2020

cc @atanasster

@atanasster
Copy link
Member

Looking great @novli , just a small issue maybe to look at

@novli novli requested a review from atanasster January 23, 2020 10:21
@chrmod
Copy link

chrmod commented Feb 1, 2020

Please make this happen, currently knob are unusable for statefull components.
It’s maybe important to point out that current behavior is very confusing for developers as it is hard to identify why state changes are not being issues correctly.
We’ve spent multiple hours debugging complex component chain only to realize the knob was the problem.

@wovalle
Copy link

wovalle commented Feb 21, 2020

Hey, just want to express my interest in this feature being released. We had to do some nasty hack to get around this problem.

Does anyone know how to use this before release?

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@shilman shilman added this to the 6.0.0 milestone Feb 21, 2020
@shilman shilman changed the title addon-knobs disableForceUpdate option Addon-knobs: Add disableForceUpdate option Feb 21, 2020
@shilman shilman merged commit 461837d into storybookjs:next Feb 21, 2020
@wiese wiese mentioned this pull request Mar 3, 2020
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-Wikibase that referenced this pull request Mar 3, 2020
So far we have no use for debounced values from knobs in storybook - we
only use few knobs and the updates seem to be quite performant.
We filed storybookjs/storybook#10019 [0] to check with the storybook
maintainers if the behavior we witnessed is as expected.
While researching we saw that work [1] was put into this area recently
so maybe more fine grained controls will be possible with the next
release, which we then can use if need arises.

[0] storybookjs/storybook#10019
[1] storybookjs/storybook#9447

Change-Id: I3ea30acb60e142dff5c9fbf4fe4cda9a40d4c990
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions that referenced this pull request Mar 3, 2020
* Update Wikibase from branch 'master'
  to e26d8240f723293ad450d38bdb0140f417b90096
  - Merge "bridge: disable value debounce for knobs in storybook"
  - bridge: disable value debounce for knobs in storybook
    
    So far we have no use for debounced values from knobs in storybook - we
    only use few knobs and the updates seem to be quite performant.
    We filed storybookjs/storybook#10019 [0] to check with the storybook
    maintainers if the behavior we witnessed is as expected.
    While researching we saw that work [1] was put into this area recently
    so maybe more fine grained controls will be possible with the next
    release, which we then can use if need arises.
    
    [0] storybookjs/storybook#10019
    [1] storybookjs/storybook#9447
    
    Change-Id: I3ea30acb60e142dff5c9fbf4fe4cda9a40d4c990
@romariclaurent
Copy link

Thanks for this! 👍

Here is a little helper using hooks to abstract away some of the complexity.

Usage

import useKnob from './useKnob'; 
import ProgressionBar from './ProgressionBar';
import { withKnobs, text } from '@storybook/addon-knobs';

export default {
  title: 'ProgressionBar',
  component: ProgressionBar,
  decorators: [withKnobs({ disableForceUpdate: true })],
};
export const Progression = (): JSX.Element => {
   const progression = useKnob('Progression', text, '25%');
   return (
      <ProgressionBar progressionValue={progression} />
   );
}

useKnob

// useKnob.ts
import addons from '@storybook/addons';
import { useEffect, useState } from 'react';
export default (name, knob, ...params) => {
  const initialValue = knob(name, ...params);
  const [value, setValue] = useState(initialValue);
  useEffect(() => {
    const onChange = (change: any) => {
      if (!change.manual) {
        if (change.name === name) {
          setValue(change.value);
        }
      }
    };
    addons.getChannel().addListener('storybookjs/knobs/change', onChange);
    return () => {
      addons.getChannel().removeListener('storybookjs/knobs/change', onChange);
    };
  }, []);
  return value;
};

@wiese
Copy link

wiese commented Apr 25, 2020

Our team got a little confused over disableDebounce and the way it is documented earlier (#10019 (comment)).

Knobs documentation on next currently makes no mention of disableForceUpdate or disableDebounce, but still(?) talks about a timestamps option, the description of with sounds strangely related.

Assuming the author of this PR is on top of what the status quo is and how things are supposed to work, it would be great for the options to be documented. Thanks!

@benbenbenbenbenben
Copy link

benbenbenbenbenben commented Aug 14, 2020

If you're using svelte and you want to do what @romariclaurent has done for react, you can try a decorator function like this:

const svelteKnobs = (story: Function) => {
    let Component = (function () {
        const C = story().Component
        const c = new C(...arguments)
        const change = (change: any) => {
            if (!change.manual) {
                const prop: any = {}
                prop[change.name] = change.value
                c.$set(prop)
            }
        }
        /* unhook event on destruction */
        c.$$.on_destroy.push(function () {
            addons.getChannel().removeListener('storybookjs/knobs/change', change)
        })
        /* hook event ~anyway~ as onMount already happened */
        addons.getChannel().addListener('storybookjs/knobs/change', change)
        return c
    })
    return ({
        ...story(),
        Component
    })
}

And then in your story:

export default {
    title: 'RenderGizmo',
    /* add the sveltKnobs decorator that knows to update individual props */
    decorators: [svelteKnobs, withKnobs({ disableForceUpdate: true })]
}

export const Basic = () => ({
    Component: RenderGizmo,
    props: {
        cvsrc,
        /* use knobs like you normally do */
        pdfPage: number('pdfPage', 1)
    }
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants